[components] Add windows testing to dagster-dg (BUILD-672) #27537

smackesey · 2025-02-04T01:52:15Z

Summary & Motivation

dagster-dg is currently broken on Windows. I found this out by trying to add Windows testing. This PR fixes dg for Windows and adds Windows testing.

Modify our azure pipeline specification to also test dagster-dg.
Make sure all paths are handled with Path and don't contain embedded "/"
Handle process shutdown appropriately on Windows for dg dev
Some adjustments to the tests of console output, since sometimes the box character used to draw tables and panels differ on Windows.

NOTE: There are still some problems with dg dev testing in the CLI environment on Windows, so the dg dev tests are skipped in this PR. They will be re-added upstack.

How I Tested These Changes

Test suite now passes on Windows.

smackesey · 2025-02-04T01:52:36Z

[components] Add windows testing to dagster-dg (BUILD-672) #27537 : 2 dependent PRs (#27710 , #27799 ) 👈 (View in Graphite)
move dagster dev tests to core #27654
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

graphite-app · 2025-02-04T16:10:22Z

python_modules/libraries/dagster-dg/dagster_dg_tests/utils.py

+
+        venv_bin = Path.cwd() / ".venv" / "bin"
+        with modify_environment_variable(
+            "PATH", os.pathsep.join([str(venv_bin), os.pathsep, os.environ["PATH"]])


The PATH construction has an extra separator due to os.pathsep being included in the join list. This creates paths like /venv/bin::/usr/bin with double separators. The correct construction should be:

os.pathsep.join([str(venv_bin), os.environ['PATH']])

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-02-07T03:27:14Z

python_modules/libraries/dagster-dg/dagster_dg_tests/cli_tests/test_custom_help_format.py

+def test_join_path():
+    from pathlib import Path
+
+    my_path = Path("foo/bar")
+    updated_path = my_path / "baz"
+    print(updated_path)
+    assert False
+


This test appears to be a debugging function that was inadvertently committed. It contains a forced failure (assert False) and debug print statements, suggesting it was used to investigate Path joining behavior. Consider removing this test or converting it into meaningful test coverage if path joining behavior needs verification.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

gibsondan · 2025-02-13T15:07:52Z

python_modules/libraries/dagster-dg/dagster_dg/cache.py

@@ -78,6 +77,7 @@ def clear_key(self, key: tuple[str, ...]) -> None:

    def clear_all(self) -> None:
        shutil.rmtree(self._root_path)
+        assert not self._root_path.exists()


just confirming this was intentional and not a local testing thing, could imagine warning or something being less disruptive if this didn't happen for some obscure filesystem reason

that actually was a testing thing, good catch

gibsondan · 2025-02-13T15:10:13Z

python_modules/libraries/dagster-dg/dagster_dg/utils/__init__.py

@@ -31,6 +32,27 @@
 CLI_CONFIG_KEY = "config"


+def is_windows() -> bool:
+    return platform.system() == "Windows"


the way we check this in dagster core is slightly different, not sure if that matters

dagster/python_modules/dagster/dagster/_seven/__init__.py

Line 22 in bc7dd71

IS_WINDOWS = os.name == "nt"

chatGPT claims sys.platform == "win32" is most robust so going with that.

gibsondan · 2025-02-13T15:10:34Z

python_modules/libraries/dagster-dg/dagster_dg/utils/__init__.py

+def is_macos() -> bool:
+    return platform.system() == "Darwin"


high confidence this covers all mac flavors?

Probably does, but changed to sys.platform == "darwin" since that is likely most robust.

gibsondan · 2025-02-13T15:11:09Z

python_modules/libraries/dagster-dg/dagster_dg/utils/__init__.py

@@ -85,10 +107,16 @@ def is_executable_available(command: str) -> bool:
    return bool(shutil.which(command)) or bool(get_uv_run_executable_path(command))


+# Short for "normalize path"-- use this to get the platform-correct string representation of an
+# existing string path.
+def npath(path: str):


i would find normalize_path clearer as a reader fwiw at the expense of your wrist tendons

Good idea-- changed to cross_platform_string_path for absolute clarity. Mouthful but there's only one callsite anyway.

smackesey marked this pull request as ready for review February 4, 2025 01:52

smackesey force-pushed the windows/components/add-windows-testing branch from c5f92c5 to 9894ca5 Compare February 4, 2025 02:00

smackesey changed the title ~~[components] Add windows testing to dagster-dg~~ [components] Add windows testing to dagster-dg (BUILD-672) Feb 4, 2025

smackesey force-pushed the sean/components/dg-dev branch from 0ad8ae0 to 81af794 Compare February 4, 2025 02:29

smackesey force-pushed the windows/components/add-windows-testing branch 4 times, most recently from 7dbac06 to 9995d6c Compare February 4, 2025 16:09

smackesey force-pushed the sean/components/dg-dev branch from 81af794 to 594bc15 Compare February 4, 2025 16:09

graphite-app bot reviewed Feb 4, 2025

View reviewed changes

smackesey force-pushed the windows/components/add-windows-testing branch from 9995d6c to f13115a Compare February 4, 2025 16:13

Base automatically changed from sean/components/dg-dev to master February 4, 2025 16:49

smackesey force-pushed the windows/components/add-windows-testing branch 12 times, most recently from f4597f5 to a17e230 Compare February 7, 2025 03:26

graphite-app bot reviewed Feb 7, 2025

View reviewed changes

smackesey force-pushed the graphite-base/27537 branch from e2d78a6 to d3c450d Compare February 10, 2025 13:37

smackesey force-pushed the windows/components/add-windows-testing branch from 25c7c84 to fb67fa3 Compare February 10, 2025 13:37

smackesey mentioned this pull request Feb 10, 2025

[components] Add --json flag for dg component-type list #27710

Merged

smackesey force-pushed the windows/components/add-windows-testing branch from fb67fa3 to bd8f823 Compare February 10, 2025 16:52

smackesey force-pushed the graphite-base/27537 branch from d3c450d to f5a7070 Compare February 10, 2025 16:52

smackesey force-pushed the windows/components/add-windows-testing branch from bd8f823 to ed918eb Compare February 12, 2025 13:37

smackesey force-pushed the graphite-base/27537 branch from f5a7070 to 1825d72 Compare February 12, 2025 13:37

smackesey changed the base branch from graphite-base/27537 to master February 12, 2025 13:37

smackesey force-pushed the windows/components/add-windows-testing branch 2 times, most recently from 3aaa2a9 to ed2eb96 Compare February 12, 2025 16:33

smackesey mentioned this pull request Feb 12, 2025

[components] Add --require-local-venv to dagster-dg #27799

Open

smackesey force-pushed the windows/components/add-windows-testing branch 7 times, most recently from c866c43 to 6633929 Compare February 12, 2025 22:21

smackesey mentioned this pull request Feb 12, 2025

[components] Reenable dg dev deployment test on windows #27815

Draft

smackesey force-pushed the windows/components/add-windows-testing branch 4 times, most recently from d1fc49e to fcb0c8f Compare February 13, 2025 14:39

gibsondan approved these changes Feb 13, 2025

View reviewed changes

smackesey force-pushed the windows/components/add-windows-testing branch 2 times, most recently from b55cf67 to 29dcb43 Compare February 13, 2025 19:25

add windows testing

00d5369

smackesey force-pushed the windows/components/add-windows-testing branch from 29dcb43 to 00d5369 Compare February 13, 2025 21:50

smackesey merged commit ad4a451 into master Feb 13, 2025
14 checks passed

smackesey deleted the windows/components/add-windows-testing branch February 13, 2025 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[components] Add windows testing to dagster-dg (BUILD-672) #27537

[components] Add windows testing to dagster-dg (BUILD-672) #27537

smackesey commented Feb 4, 2025 •

edited

Loading

smackesey commented Feb 4, 2025 •

edited

Loading

graphite-app bot Feb 4, 2025

graphite-app bot Feb 7, 2025

gibsondan Feb 13, 2025

smackesey Feb 13, 2025

gibsondan Feb 13, 2025

smackesey Feb 13, 2025

gibsondan Feb 13, 2025

smackesey Feb 13, 2025

gibsondan Feb 13, 2025

smackesey Feb 13, 2025

[components] Add windows testing to dagster-dg (BUILD-672) #27537

[components] Add windows testing to dagster-dg (BUILD-672) #27537

Conversation

smackesey commented Feb 4, 2025 • edited Loading

Summary & Motivation

How I Tested These Changes

smackesey commented Feb 4, 2025 • edited Loading

graphite-app bot Feb 4, 2025

Choose a reason for hiding this comment

graphite-app bot Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smackesey commented Feb 4, 2025 •

edited

Loading

smackesey commented Feb 4, 2025 •

edited

Loading